Skip to content

refactor(mercury): remove unnecessary array allocation and copy in decode_pack_object#755

Merged
genedna merged 2 commits into
gitmono-dev:mainfrom
el-ev:avoid_vec_copy_2
Dec 14, 2024
Merged

refactor(mercury): remove unnecessary array allocation and copy in decode_pack_object#755
genedna merged 2 commits into
gitmono-dev:mainfrom
el-ev:avoid_vec_copy_2

Conversation

@el-ev

@el-ev el-ev commented Dec 14, 2024

Copy link
Copy Markdown
Contributor

The lambda function does not work as it is described. And it introduces unnecessary memory and time overhead.
It says that it keeps track of the new Object indexed by the Delta object. But that new Object will be created by Pack::rebuild_delta(), and well recorded in process_delta():

let mut new_obj = Pack::rebuild_delta(delta_obj, base_obj);
new_obj.set_mem_recorder(shared_params.cache_objs_mem_size.clone());
new_obj.record_mem_size();

The memory used by the Delta object, which is said to be the size of the new Object, is released after Pack::rebuild_delta() returns.

During the whole lifetime of the Delta object, the new Object does not even exist in the memory, so it's meaningless to keep track of it. Instead, the memory used by the Delta object itself should be recorded.

@vercel

vercel Bot commented Dec 14, 2024

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mega ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 14, 2024 11:20am

@genedna
genedna added this pull request to the merge queue Dec 14, 2024
Merged via the queue into gitmono-dev:main with commit 1329e67 Dec 14, 2024
@el-ev
el-ev deleted the avoid_vec_copy_2 branch December 14, 2024 11:56
@MrBeanCpp

Copy link
Copy Markdown
Contributor

@el-ev Hello, the reserve_delta_data lambda function was specifically introduced for precise memory management, and it should work as intended. It's purpose is to handle memory accurately because delta objects expand in memory usage after rebuild_delta() (delta object → new object).

A good analogy would be placing a group of pufferfish into a fixed-volume container. When the pufferfish inflate, the container's lid inevitably pops open due to the increased volume.

Similarly, when memory_used() exceeds the upper limit (e.g., 2GB), we block the input data stream. However, at this point, a considerable number of delta objects remain in memory. Once rebuild_delta() is executed, the program’s memory usage will increase significantly beyond 2GB, invalidating memory control and potentially leading to an OOM kill.

For this reason, we must accurately estimate the final memory consumption of delta objects right from the beginning.

So, reserve_delta_data is indeed necessary.

It says that it keeps track of the new Object indexed by the Delta object. But that new Object will be created by Pack::rebuild_delta(), and well recorded in process_delta()

The memory of delta objects is tracked after decode_pack_object, while the memory of new objects is tracked after rebuild_delta, replacing the original delta objects.

let r: Result<CacheObject, GitError> = self.decode_pack_object(&mut reader, &mut offset);
match r {
    Ok(mut obj) => {
        obj.set_mem_recorder(self.cache_objs_mem.clone());
        obj.record_mem_size();

My goal is to ensure that this transformation does not disrupt memory control. Since we cannot just kick out objects before they reach the cache to reduce memory usage, accurate pre-estimation of memory consumption becomes critically important.

Imagine a small, isolated island trying to manage its population knowing that new members will inevitably be born. Without careful planning and foresight, overcrowding would be unavoidable.

During the whole lifetime of the Delta object, the new Object does not even exist in the memory, so it's meaningless to keep track of it. Instead, the memory used by the Delta object itself should be recorded.

The delta object will eventually become a new object, so estimating its final memory usage is essential to prevent memory from exceeding expectations.

@el-ev

el-ev commented Dec 14, 2024

Copy link
Copy Markdown
Contributor Author

@MrBeanCpp I understand your consideration now, and I will push some fix for the issue (ideally not simply revert the commit).

However, I can't agree that pre-creating a much larger vector and do heavy coipes is the proper way to limit the memory usage. It is worth noting that HeapSize is implemented by the developer. We could modify the implementation to take the actual memory usage into account:

fn heap_size(&self) {
    if &self.is_delta() {
        self.data_decompress.heap_size() + self.expanded_size
    } else {
        self.data_decompress.heap_size()
    }
}

I think it could satisty your requirement, without huge runtime overhead.

@MrBeanCpp

Copy link
Copy Markdown
Contributor

Sounds good, you can monitor the memory usage during the decoding process to evaluate the effectiveness of memory control.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants